Repository navigation
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
When two or more vLLM services share a node, each now gets its own CUDA_VISIBLE_DEVICES (TP x PP x instances per node, in declaration order). A ray service there gets the leftover GPUs. A node with one GPU service renders nothing new. A manual CUDA_VISIBLE_DEVICES index list wins and reserves its GPUs. Overlaps and over-subscription fail validation. `share_gpus: true` opts a service out. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
…vLLM beside Ray Slurm resets CUDA_VISIBLE_DEVICES to the step's GPUs, so a value passed as `env K=V srun` never reaches the task. Checked on a 4-GPU GB200 node: the env-prefix value came back as 0,1,2,3, while `export` inside the step's command left CUDA seeing only the planned GPUs. Planned and manual values on a packed node are now exported in the step. A lone vLLM service beside a GPU-using ray service is now packed too, so Ray no longer advertises the model's GPUs. A ray service with num_gpus: 0, or a single GPU service on a node, still renders nothing new. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
A pool with `nodes: auto` is sized by Gym. The policy goes on the pool's first node, beside the driver. The other vLLM services go biggest first into the first node with room, never split. A service bigger than one node gets ceil(need / gpus_per_node) whole nodes of its own. More than one node splits the pool into sub-pools `<pool>-<i>`, placed with the existing node-pool --nodelist mechanism; a ray service spanning the pool spans them all and gets each node's leftover GPUs. Explicit node counts are unchanged. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
A service's own CUDA_VISIBLE_DEVICES was passed as `env K=V srun`, which Slurm resets to the step's GPUs, so it was silently ignored. It is now exported inside the step's command on every node, including a node with a single service. A runtime:VAR value is relayed as GYM_CUDA_VISIBLE_DEVICES so the step still exports the batch shell's $VAR. Drop runtime:CUDA_VISIBLE_DEVICES from the multi-instance example: it re-exported the job's full GPU set and did nothing. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
A prefill/decode tier owns its nodes, so a pool holding one is never packed, and the vllm-router takes no GPUs. `nodes: auto` refuses a pool a vllm_pd service uses: P/D pools need explicit node counts for now. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
… lands on Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
7848acf to
62ecf03
Compare
prokotg
left a comment
There was a problem hiding this comment.
We discussed that information about gpus needed should flow from services and pools should be user-facing concepts with no pools modified.
There might be a problem with how pd vllm service is placed but we consider it outside of the scope of this PR so anything is acceptable
| class NodePool(_StrictModel): | ||
| partition: str | ||
| nodes: int = 1 | ||
| # "auto": Gym packs the pool's vLLM services onto as few nodes as fit (see _resolve_auto_pool). |
There was a problem hiding this comment.
That's pretty slick but I want to highlight that this option might blow out when someone makes a mistake in services. It's not to say we shouldn't have auto mode but that we accept the risk
| "size it by. Pin a service to it with node_pool, or give it an explicit node count." | ||
| ) | ||
|
|
||
| def need(service: VllmServiceConfig) -> int: |
| def _manual_gpus(service: BaseServiceConfig) -> list[int] | None: | ||
| value = service.env.get(_CUDA_VISIBLE_DEVICES) | ||
| if value is None or not _GPU_INDEX_LIST_RE.match(value): | ||
| return None | ||
| return [int(index) for index in value.split(",")] |
There was a problem hiding this comment.
Question about assumptions: how would that work for asymmetrical multi-node jobs ? do we assume jobs will be symmetrical, meaning each node assigned to the job will use the same number of gpus?
| def _gpus_needed_per_node(service: VllmServiceConfig, nodes: int, gpus_per_node: int) -> int | None: | ||
| """GPUs this service uses on each of its nodes, or None when Ray places it across nodes.""" | ||
| tp_pp = service.tensor_parallel_size * service.pipeline_parallel_size | ||
| if nodes <= 1: | ||
| return tp_pp * service.number_of_instances | ||
| if service.number_of_instances > 1 and not effective_ray_serve(service, nodes, [gpus_per_node]): | ||
| return tp_pp * (service.number_of_instances // nodes) | ||
| return None |
There was a problem hiding this comment.
In my view, this function should be per-service-class property and the process that plans gpus should read each service property to make a call
| def plan_gpus(config: "SubmitConfig") -> dict[str, dict[str, str]]: | ||
| """The CUDA_VISIBLE_DEVICES to render, keyed by node pool and then service. | ||
|
|
||
| A pool is planned only when a vLLM service on it shares it with another service that wants |
There was a problem hiding this comment.
I know vLLM service began discussion about the automatic placement but this might be confusing for anyone reading. IIUC this should be service-agnostic
| # vLLM doesn't require auth; dummy key satisfies clients that require the header. | ||
| benchmark.run["policy_api_key"] = "dummy" # pragma: allowlist secret | ||
|
|
||
| plan_gpus(self) |
| return None | ||
|
|
||
|
|
||
| def plan_gpus(config: "SubmitConfig") -> dict[str, dict[str, str]]: |
There was a problem hiding this comment.
I think plan_gpus should live outside of API
| The policy goes first, on the pool's first node, where the driver reaches it on localhost. | ||
| The rest go biggest first into the first node with room. A service bigger than one node | ||
| gets ceil(need / gpus_per_node) whole nodes to itself. More than one node splits the pool | ||
| into sub-pools named `<pool>-<i>`, which the rest of the executor places like any pool. |
There was a problem hiding this comment.
Compute Pool should be a user-facing concept and we should not alter it. It's a declaration of compute user has. Executors should learn how to place services within a pool.
| for pool_name, pool in pools.items(): | ||
| if pool.gpus_per_node is None: | ||
| continue | ||
| on_pool = { | ||
| name: service | ||
| for name, service in config.deployed_services.items() | ||
| if single_node | ||
| or ( | ||
| pool_name in (*service.node_pools, driver_pool) | ||
| if isinstance(service, RayServiceConfig) | ||
| else service.node_pool == pool_name | ||
| ) | ||
| } | ||
| if any(isinstance(s, VllmPDTierConfig) for s in on_pool.values()): | ||
| continue | ||
| claimants = { | ||
| n: s | ||
| for n, s in on_pool.items() | ||
| if isinstance(s, (VllmServiceConfig, RayServiceConfig)) and _takes_own_gpus(s) | ||
| } | ||
| vllms = {n: s for n, s in claimants.items() if isinstance(s, VllmServiceConfig)} | ||
| if not vllms or len(claimants) < 2: | ||
| continue | ||
| needs = {n: _gpus_needed_per_node(s, pool.nodes, pool.gpus_per_node) for n, s in vllms.items()} | ||
| if None in needs.values(): | ||
| continue | ||
| manual = {n: gpus for n, s in claimants.items() if (gpus := _manual_gpus(s)) is not None} | ||
| needs.update({n: len(gpus) for n, gpus in manual.items()}) |
There was a problem hiding this comment.
Can I offer to simplify gpu request discovery? Instead of differentiating on different services within an external function based on subpools, I believe the information about the service resource shape and number should come directly from the service:
A property gpus_needed on BaseServiceConfig replaces the vLLM-specific logic:
- 0 for share_gpus or non-GPU services
len(manual)for a hand-setCUDA_VISIBLE_DEVICES- otherwise the subclass's count (for vLLM, TP × PP × instances)
- place_services replaces
plan_gpusand does both node-level and GPU-level packing, withoutisinstance(VllmServiceConfig)checks.
| partition: str | ||
| nodes: int = 1 | ||
| # "auto": Gym packs the pool's vLLM services onto as few nodes as fit (see _resolve_auto_pool). | ||
| nodes: int | Literal["auto"] = 1 |
There was a problem hiding this comment.
This comes directly from my comment on sub-pools but "auto" should resolve to cumulative number of nodes
Follow-up to the #3654 review (#3654 (comment)): automatic GPU placement, agreed to land separately. Replaces #3707 (closed), rebased onto main.
The problem
Every service on a node sees every GPU. A 1-GPU judge and a 2-GPU policy on one 4-GPU node both start on GPU 0. Today the only fix is a hand-written
CUDA_VISIBLE_DEVICESper service.What changed
CUDA_VISIBLE_DEVICES(TP x PP x instances), lowest free GPUs first, in declaration order. A Ray service gets the leftovers.env K=V srunvalue to all GPUs; anexportinside the step survives. A manualCUDA_VISIBLE_DEVICESnow takes effect too, and its GPUs count as taken.share_gpus: true. Overlapping manual sets or over-subscription fail at config validation with a clear error.nodes: auto. A pool sized by Gym: the policy goes on the first node, other services biggest-first into the first node with room, never split.What stays the same
vllm_pd: a pool holding a prefill/decode tier is never packed, the router takes no GPUs, and the job script is byte-identical to main.nodes: autorefuses a P/D pool.Out of scope
Ray-placed multi-node services, an unpinned vLLM service in a multi-node job,
nodes: autofor P/D pools, and moving services between explicitly sized pools.Testing
tests/unit_tests/test_slurm_script.py: disjoint sets, manual wins, errors, opt-out, Ray leftovers, per-pool packing, in-step export,nodes: auto, andvllm_pdbyte-identity. All 495 orchestration tests pass; ruff clean.nodes: autotake a P/D pool, or probing everything on localhost each fails at least one test.Cluster evidence (at
7848acfe4, before a conflict-free rebase onto main; oci-hsg, 4-GPU GB200,gym eval submit, Qwen3-4B, AIME25 subset)nodes: 1, policy TP2 + judge TP1nvidia-smi). 96/96 rollouts, no OOM.nodes: auto, 6 GPUs of needvllm_pd, 1 prefill + 1 decode node